Change client table from Log to Set by guoyuhong · Pull Request #4598 · ray-project/ray

guoyuhong · 2019-04-10T12:28:56Z

What do these changes do?

Change client table from Log to Set.
We can totally delete the dead client entries from client table. However, we'd better keep information the clients that were connected before. Thus, I will remove the first entry that is added during the connection time. The 2-entry problem for a dead client caused many problems, so this change will make it clear.
Since we do not use insertion operation, is_insertion is changed to is_connected which is more meaningful.

Related issue number

Linter

I've run scripts/format.sh to lint the changes in this PR.

AmplabJenkins · 2019-04-10T12:36:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/415/
Test PASSed.

AmplabJenkins · 2019-04-10T13:10:05Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13696/
Test FAILed.

guoyuhong · 2019-04-10T16:19:00Z

It looks like when starting 2 node in the test, the driver will connect to a random node now... Some tests will fail in this case...

AmplabJenkins · 2019-04-11T12:27:41Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/429/
Test PASSed.

AmplabJenkins · 2019-04-11T14:50:10Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13730/
Test FAILed.

guoyuhong · 2019-04-12T06:06:00Z

I think we'd better add a RAY.SET_REPLACE command to guarantee the atomic of the replace operation.

raulchen · 2019-04-12T09:21:15Z

python/ray/tests/test_basic.py

This would be simpler.

node = cluster.add_node(num_cpus=3, resources={"CustomResource": 1}) available_plasma_socket_name = node.plasma_store_socket_name

@raulchen Thanks! That is much better!

AmplabJenkins · 2019-04-12T12:23:02Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/457/
Test PASSed.

AmplabJenkins · 2019-04-12T14:33:18Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13770/
Test FAILed.

AmplabJenkins · 2019-04-12T14:53:07Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13771/
Test FAILed.

guoyuhong · 2019-04-14T03:26:14Z

@hartikainen I have refined the temporary code which is used to fix #4140 . Could you take a look and test whether this fix is still working?

AmplabJenkins · 2019-04-14T03:28:38Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/476/
Test PASSed.

AmplabJenkins · 2019-04-14T05:49:30Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13791/
Test FAILed.

guoyuhong · 2019-04-15T12:54:43Z

@kfstorm Could you take a look?

raulchen · 2019-04-15T13:00:24Z

src/ray/gcs/tables.cc

What if multiple nodes call replace at the same time? Will there be race conditions?

Yes, the monitor and the raylet may both try to change the status. I will change the Log level from FATAL to ERROR.

raulchen · 2019-04-15T13:03:36Z

src/ray/gcs/tables.cc

typo ignore

kfstorm

Can you explain why "we'd better keep information the clients that were connected before"? I think the code is still complicated with the is_connected flag.

kfstorm · 2019-04-16T02:38:34Z

src/ray/gcs/redis_module/ray_redis_module.cc

Should we consider about the scenario that LRU has removed the whole key before replace happens?

I have replace the FATAL log to ERROR, so this may not cause crash even if the LRU mechanism has removed this entry.

kfstorm · 2019-04-16T07:04:08Z

src/ray/gcs/tables.cc

Does this have something to do with profile table?

Good catch! This is totally wrong to use CreateProfileTableData. However, because ProfileTableData and SetReplaceEntryData have similar structure, there is no error caught.

kfstorm · 2019-04-16T07:09:44Z

python/ray/experimental/state.py

typo: exists

AmplabJenkins · 2019-04-25T04:35:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/592/
Test PASSed.

guoyuhong · 2019-04-25T05:42:02Z

@kfstorm If we do not keep that information, there is no way to know whether a node was connected or not. It's unlike the object table, we can reconstruct a missing object, but for client, this information is important.

AmplabJenkins · 2019-04-25T05:49:49Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/593/
Test PASSed.

AmplabJenkins · 2019-04-25T07:05:11Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13961/
Test FAILed.

AmplabJenkins · 2019-04-25T08:19:58Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/13962/
Test FAILed.

guoyuhong · 2019-05-07T04:43:22Z

@kfstorm @raulchen @ste Do you have more comments?

stephanie-wang

Sorry for the late review. This looks good, thanks!

stephanie-wang · 2019-05-07T17:52:55Z

python/ray/tests/conftest.py

I think the previous version of this was okay (cluster.redis_address should not change).

stephanie-wang · 2019-05-07T18:08:07Z

src/ray/gcs/redis_module/ray_redis_module.cc

This flag is awkward. Can we remove it and just have this function always return REDISMODULE_OK? I would prefer if we just had the caller explicitly check the value and call RedisModule_ReplyWithSimpleString(ctx, "OK") if necessary. We can make the pattern a macro if it becomes cumbersome.

I remove the flag and change the function return value to Status. Then we can call the reply function at the top layer.

stephanie-wang · 2019-05-07T18:11:54Z

src/ray/gcs/redis_module/ray_redis_module.cc

I think it is safer to not allocate a new variable called new_argv and instead pass in argv_vector.data() here. Also, I would probably then rename argv_vector to new_argv or something.

stephanie-wang · 2019-05-07T18:33:16Z

src/ray/gcs/tables.cc

The notification_mode should always be GcsTableNotificationMode::APPEND_OR_ADD, right? Can we make this a check instead of an if?

We cannot do that, this is a callback for RequestNotifications. There will be messages for both APPEND_OR_ADD and REMOVE.

src/ray/gcs/tables.cc

src/ray/gcs/tables.h

kfstorm · 2019-05-08T07:50:12Z

LGTM.

It will be better if we have a Map structure in future days. 😁

AmplabJenkins · 2019-05-19T10:16:49Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14330/
Test FAILed.

guoyuhong · 2019-05-19T12:15:58Z

@raulchen I have resolved the conflict. It looks like that we need to do some code change to make this work with dynamic resources. It looks like I need to move this code snippet into node manager. And every time the resource updating request is received, the backend needs to look up GCS first and then change the resource correspondingly and then call Set::Replace.

AmplabJenkins · 2019-05-19T14:16:04Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/14333/
Test FAILed.

AmplabJenkins · 2019-05-20T09:24:46Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/865/
Test FAILed.

AmplabJenkins · 2019-05-20T16:51:33Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-Perf-Integration-PRB/868/
Test FAILed.

guoyuhong · 2019-06-03T02:52:13Z

Since Hash Table is preferred and there are racing condition in dynamic resource while using Set, this PR will be closed. The hash table PR is #4911 .

guoyuhong force-pushed the clientTable2set branch from 8a47230 to c6fc966 Compare April 11, 2019 12:22

raulchen reviewed Apr 12, 2019

View reviewed changes

guoyuhong force-pushed the clientTable2set branch from c3bca01 to 0a4b9ee Compare April 14, 2019 03:24

guoyuhong requested a review from hartikainen April 14, 2019 03:27

raulchen reviewed Apr 15, 2019

View reviewed changes

raulchen requested a review from stephanie-wang April 15, 2019 13:11

kfstorm reviewed Apr 16, 2019

View reviewed changes

guoyuhong force-pushed the clientTable2set branch from 0a4b9ee to 0a6467e Compare April 25, 2019 04:33

raulchen approved these changes May 7, 2019

View reviewed changes

stephanie-wang requested changes May 7, 2019

View reviewed changes

Squash commits to one

ec9209c

Resolve conflict and fix gtest cass

65f96a0

guoyuhong force-pushed the clientTable2set branch from 8efb62a to 65f96a0 Compare May 19, 2019 12:09

guoyuhong closed this Jun 3, 2019

guoyuhong deleted the clientTable2set branch June 3, 2019 02:52

Conversation

guoyuhong commented Apr 10, 2019

What do these changes do?

Related issue number

Linter

Uh oh!

AmplabJenkins commented Apr 10, 2019

Uh oh!

AmplabJenkins commented Apr 10, 2019

Uh oh!

guoyuhong commented Apr 10, 2019

Uh oh!

AmplabJenkins commented Apr 11, 2019

Uh oh!

AmplabJenkins commented Apr 11, 2019

Uh oh!

guoyuhong commented Apr 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 12, 2019

Uh oh!

AmplabJenkins commented Apr 12, 2019

Uh oh!

AmplabJenkins commented Apr 12, 2019

Uh oh!

guoyuhong commented Apr 14, 2019

Uh oh!

AmplabJenkins commented Apr 14, 2019

Uh oh!

AmplabJenkins commented Apr 14, 2019

Uh oh!

guoyuhong commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfstorm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Apr 25, 2019

Uh oh!

guoyuhong commented Apr 25, 2019

Uh oh!

AmplabJenkins commented Apr 25, 2019

Uh oh!

AmplabJenkins commented Apr 25, 2019

Uh oh!

AmplabJenkins commented Apr 25, 2019

Uh oh!

guoyuhong commented May 7, 2019

Uh oh!

stephanie-wang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!